token-cli: add command for updating metadata address #4810
Conversation
sanjsingh07
changed the title
There was a problem hiding this comment.
Thanks for your contribution! A few bits to firm up the interface, and also, could you add a test for this? You can use the existing metadata_pointer() test for creating, and then just add a step where you update the metadata address, and check that the change was properly propagated.
token/cli/src/main.rs
Outdated
| .arg(owner_address_arg()) | ||
| .arg(multisig_signer_arg()) | ||
| .nonce_args(true) | ||
| .offline_args(), |
There was a problem hiding this comment.
Can remove the offline args, since the instruction doesn't support sign-only
token/cli/src/main.rs
Outdated
| Defaults to the client keypair.", | ||
| ), | ||
| ) | ||
| .arg(owner_address_arg()) |
There was a problem hiding this comment.
You're using the authority arg and not the owner arg, so this can be removed
token/cli/src/main.rs
Outdated
| .validator(is_valid_signer) | ||
| .takes_value(true) | ||
| .help( | ||
| "Specify the token's authority. \ |
There was a problem hiding this comment.
nit: Let's be clear
| "Specify the token's authority. \ | |
| "Specify the token's metadata-pointer authority. \ |
token/cli/src/main.rs
Outdated
| .arg( | ||
| Arg::with_name("metadata_address") | ||
| .long("metadata-address") | ||
| .value_name("ADDRESS") | ||
| .takes_value(true) | ||
| .help( | ||
| "Specify address that stores token metadata." | ||
| ), | ||
| ) |
There was a problem hiding this comment.
This will introduce a footgun. As it stands, if someone just accidentally forgets to put in a metadata address, the command will clear it.
Instead, this should look like the new_authority flag in Authorize, where someone can specify --disable to clear the metadata pointer address. Then if --disable is not provided, the metadata_address argument is required.
token/cli/src/main.rs
Outdated
| @@ -3624,6 +3652,44 @@ fn app<'a, 'b>( | |||
| .nonce_args(true) | |||
| .offline_args(), | |||
| ) | |||
| .subcommand( | |||
| SubCommand::with_name(CommandName::UpdateMetadataPointerAddress.into()) | |||
| .about("Updates default account state for the mint. Requires the default account state extension.") | |||
There was a problem hiding this comment.
| .about("Updates default account state for the mint. Requires the default account state extension.") | |
| .about("Updates metadata pointer address for the mint. Requires the metadata pointer extension.") |
token/cli/src/main.rs
Outdated
| .takes_value(true) | ||
| .index(1) | ||
| .required(true) | ||
| .help("The address of the token mint to update default account state"), |
There was a problem hiding this comment.
| .help("The address of the token mint to update default account state"), | |
| .help("The address of the token mint to update the metadata pointer address"), |
token/cli/src/main.rs
Outdated
| @@ -144,6 +144,7 @@ pub enum CommandName { | |||
| EnableCpiGuard, | |||
| DisableCpiGuard, | |||
| UpdateDefaultAccountState, | |||
| UpdateMetadataPointerAddress, | |||
There was a problem hiding this comment.
nit for ease of use
| UpdateMetadataPointerAddress, | |
| UpdateMetadataAddress, |
joncinque
changed the title
|
updated |
| ) | ||
| .arg( | ||
| Arg::with_name("metadata_address") | ||
| .index(2) |
There was a problem hiding this comment.
Can you add .validator(is_valid_pubkey) for this arg too? Otherwise, if someone inputs an invalid pubkey, it'll disable the metadata pointer address!
There was a problem hiding this comment.
Yupp, Done! and also sorry about this many commits. :)
There was a problem hiding this comment.
Looks great, thanks for your patience! I'll ping you if CI has any issues.
And a lot of commits is really good, it makes it much easier for me to review. Do avoid merge commits in the future though, and prefer rebasing + force-pushing your branch whenever you need to.
|
All good, thanks for your work! |
resolved #4701